build(COMPASS-28): move to js-yaml 5 and ajv-formats 3 - #2721
Conversation
Automated review —
|
minimumReleaseAgeExclude shape |
resolves |
|---|---|
"@airtasker/*" (scalar — repo today) |
js-yaml@5.3.0 |
YAML list (- "@airtasker/*") |
js-yaml@5.2.3 |
| removed entirely | js-yaml@5.2.3 |
# current — parses as a str, and the whole gate goes quiet
minimumReleaseAgeExclude: "@airtasker/*"
# fix
minimumReleaseAgeExclude:
- "@airtasker/*"Consequence: the 7-day quarantine has been off repo-wide, not just for js-yaml. That's a supply-chain control that reads as present and does nothing — worse than absent, because it invites trust. The lockfile pin in this PR holds the line for one package and expires the moment 5.3.0 clears the window legitimately.
Also corrected: 5.3.0 was 5.8 days old at commit time, not "four days" as the description says.
🟡 Important (0)
None.
🔵 Suggestions (1)
lib/src/validation-server/verifications/contract-mismatcher.ts:416 — the comment says addFormats "adds the format validators to the instance and returns it", which is true but not the whole mutation: with no options it also registers the formatMaximum/formatMinimum/formatExclusiveMaximum/formatExclusiveMinimum keywords and sets ajv.opts.code.formats. The durable fact for the next reader is the same instance is returned — that's what makes wrapping the constructor safe.
✅ Verified sound
- The rename is correct and complete. One import, one call site, no other
ajv-formatsreference in the tree;addFormats(ajv)does register-and-return (checked againstajv-formats@3.0.1source, which endsreturn ajv). - Dropping
@types/js-yamlcarries no risk. js-yaml 5 shipsdist/js-yaml.d.tswith the same named exports;esModuleInteropplus no__esModulein the CJS bundle makesimport YAML from "js-yaml"sound at both type and runtime level. - Output parity independently reproduced beyond what this PR claimed: v4.3.1 vs v5.2.3
dump(..., {skipInvalid: true})byte-identical across 18 edge cases (folding, anchors, quoting, dates, unicode, indicator chars), not just the generator fixtures. - The comment states current behaviour rather than narrating the rename, and carries no ticket shortcode.
Recommended action
- Fix the
minimumReleaseAgeExcludeshape — one line, and it restores a control that is currently inert across the whole repo. Either here or in a linked follow-up; it should not merge as undocumented prose. - Optionally sharpen the
addFormatscomment to lead with "returns the same instance".
`js-yaml` to ^5.2.3 and `ajv-formats` to ^3.0.1. `@types/js-yaml` is removed: js-yaml 5 ships its own declarations. `minimumReleaseAgeExclude` was a string where pnpm wants a list, and pnpm reads a scalar there as no exclusions and stops applying `minimumReleaseAge` altogether. So the seven-day quarantine has been covering nothing, for every package, not only this one. That is why `^5.2.3` resolved 5.3.0, five days old at the time. With the list form a fresh resolution of the same range picks 5.2.3, and the exclusion still parses. Already-pinned versions are not re-resolved, so no lockfile in the stack moves. Emitted output is unchanged. All seven generator and language combinations are byte-identical across the js-yaml major, which is the bump that could have altered quoting or indentation in every `-l yaml` artifact. `ajv-formats` was imported as `Ajv` and called as `Ajv(...)`, though it is `addFormats` — it takes an Ajv instance, registers the format validators on it and returns it. Renamed to what it is, since the next reader of a line that constructs a validator inside a call to `Ajv` has to open the package to find out which one is which. Format registration is what an ajv-formats major can silently break, and a body that fails only on a format keyword would otherwise validate clean. It is already covered: the request-body date test asserts ajv's own `must match format "date"` text, so the formats have to be registered for it to pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
20845f0 to
c1123a5
Compare
|
Addressed in |
Automated review, round 2 —
|
| Check | Result |
|---|---|
list form, ^5.2.3 |
5.2.3 |
| scalar form, same range | 5.3.0 — the round-1 diagnosis holds |
| exclusion still honoured | Yes — ["js-yaml"] → 5.3.0; multi-entry lists work |
| scoped-glob semantics intact | Yes — ["@types/*"] matched @types/node; ["@airtasker/*"] correctly did not |
pnpm install --frozen-lockfile |
succeeds |
| full re-resolution with the gate live | exit 0, zero refusals — nothing currently pinned is refused |
| suite | 555 tests / 44 snapshots pass |
skipInvalid still supported in js-yaml 5 |
yes (js-yaml.d.ts:333) |
| byte-identical YAML claim | independently reproduced across all three generators |
That second-to-last row is the one that matters for merge confidence: turning the gate on does not retroactively refuse anything already in the lockfile.
The addFormats rename and its comment are accurate, and covered by the passing must match format "date" assertion.
Recommended action
- Raise the fix upstream in the synced template, and name a follow-up ticket at the site here so a sync-driven revert is recognisable rather than invisible.
- Reword the commit message to state the invariant rather than the version — it stops being true tomorrow.
- Decide whether the 5.2.3 lockfile pin should stay once the gate permits 5.3.0.
1 participant
Ticket
COMPASS-28 — Bump
spot's NodeJS minimum from 18 to 22What
js-yamlajv-formats@types/js-yamlSupersedes Dependabot #2696 and #2255.
A supply-chain control that was silently inert
js-yamlis resolved at 5.2.3, not the 5.3.0 that^5.2.3also permits.My first read of this blamed pnpm for ignoring
minimumReleaseAge. That was wrong. The setting was never being applied, because the line below it is malformed:pnpm reads a scalar there as no exclusions and stops applying
minimumReleaseAgealtogether. Differential on the repo's own pnpm 10.28.1, spec^5.2.3:minimumReleaseAgeExcludeshape"@airtasker/*"(scalar)So the seven-day quarantine has been covering nothing, for every package — a control that reads as present and does less than absent, because it invites trust.
Fixed here as a list. A fresh resolution of the same range now picks 5.2.3, the exclusion still parses, and already-pinned versions are not re-resolved — verified no lockfile in the stack moves. That is also why the lockfile pin stays: it is what holds the line for this package until the config fix reaches
master.Corrected too: 5.3.0 was 5.8 days old at commit time, not "four days" as this description first said.
Emitted output is unchanged
js-yaml 5 is the bump most able to change every
-l yamlartifact — quoting, indentation, line folding. It did not:All seven generator × language combinations from
test-fixtures/contract/api.tsare byte-identical across the major.ajv-formatswas imported under the wrong nameThe default export is
addFormats— it takes an Ajv instance, registers the format validators on it, and returns it. The old name made the call site read as a constructor wrapping another constructor:Behaviour is unchanged; this was always
addFormats.Format registration is the one thing an ajv-formats major can break silently — a body that is wrong only on a
formatkeyword would validate clean, and bothdateanddate-timeare emitted by the JSON Schema generator. It is already covered:contract-mismatcher.spec.ts's request-body date test asserts ajv's own error text verbatim —— so the formats must be registered for that test to pass. I also probed
addFormats(new Ajv())directly and confirmed it accepts validdate/date-timevalues, rejects invalid ones, and returns the instance. No new test needed; the existing one is the right shape.How this was verified
Locally on Node 22.23.2, all exit 0:
pnpm installpnpm buildpnpm testpnpm lint:checkpnpm build-docsdocker build+check-image-parityvalidate/generate -l yaml/--help